feat(core): define module registry and lifecycle hooks#1925
Conversation
Router image scan passed✅ No security vulnerabilities found in image: |
kaialang
force-pushed
the
kaialang/mod-1-introduce-pluggable-module-registry-and-lifecycle-hooks
branch
from
de5ad92 to
3be03ff
Compare
kaialang
force-pushed
the
kaialang/mod-1-introduce-pluggable-module-registry-and-lifecycle-hooks
branch
3 times, most recently
from
98b8b8d to
364fedc
Compare
kaialang
force-pushed
the
kaialang/mod-1-introduce-pluggable-module-registry-and-lifecycle-hooks
branch
from
364fedc to
6ab8955
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change introduces a new modular plugin system ("ModulesV1") to the router framework, enabling registration, prioritization, lifecycle management, and hook-based extensibility for modules. It adds core infrastructure for module registration, ordered execution of lifecycle hooks, and context interfaces for each hook phase. Supporting utilities such as a generic ordered set and pointer helper are included. Comprehensive tests validate module lifecycles, error handling, and hook execution. Integration with the router ensures modules are initialized and cleaned up alongside existing components. Changes
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
Bot
added
studio
monorepo
controlplane
cli
protocol
connect
graphqlmetrics
aws-lambda-router
labels
kaialang
force-pushed
the
kaialang/mod-1-introduce-pluggable-module-registry-and-lifecycle-hooks
branch
2 times, most recently
from
0b0d80d to
4dfae18
Compare
github-actions
Bot
removed
studio
monorepo
controlplane
cli
protocol
connect
graphqlmetrics
labels
kaialang
changed the base branch from
topic/kaialang-eng-1063-open-core-module-system-v1
to
main
kaialang
force-pushed
the
kaialang/mod-1-introduce-pluggable-module-registry-and-lifecycle-hooks
branch
from
4dfae18 to
1b1e5f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
router/core/modules_v1.go (3)
45-61: Clean interface design with proper context support!The ModuleV1 interface and context maintain feature parity with the legacy system while providing a cleaner API. Good that both Provision and Cleanup receive context for timeout control.
136-172: Comprehensive module initialization!The initialization properly provisions modules, registers all hook types, and provides detailed logging with module IDs and timing information.
174-188: Robust cleanup with error aggregation!The cleanup implementation ensures all modules get a chance to clean up their resources, properly aggregating any errors that occur.
🧹 Nitpick comments (3)
router/internal/utils/ordered_set.go (1)
25-36: Consider using copy() to avoid potential slice reallocation.The current implementation works correctly, but using
copy()to shift elements left is more efficient as it avoids potential slice reallocation.func (s *OrderedSet[T]) Remove(elem T) { if _, exists := s.index[elem]; exists { delete(s.index, elem) // rebuild slice without the removed element for i, v := range s.elements { if v == elem { - s.elements = append(s.elements[:i], s.elements[i+1:]...) + copy(s.elements[i:], s.elements[i+1:]) + s.elements = s.elements[:len(s.elements)-1] break } } } }router/core/hooks_context.go (1)
3-3: Enhance the comment to be more descriptive.The comment could better describe the purpose and design pattern.
-// context interface for every hook +// Hook context interfaces provide type-safe parameters for lifecycle hooks. +// Each interface embeds RequestContext to provide access to common request data.router/core/hooks.go (1)
138-144: Fix typo in comment.// moduleHook is a wrapper around a hook that includes the module ID. -// this is used for tracability in case of hook execution errors. +// this is used for traceability in case of hook execution errors. type moduleHook[H any] struct {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
router-tests/modules_v1/custom_modules/database_module.go(1 hunks)router-tests/modules_v1/module_lifecycle_test.go(1 hunks)router/core/hooks.go(1 hunks)router/core/hooks_context.go(1 hunks)router/core/hooks_test.go(1 hunks)router/core/modules_v1.go(1 hunks)router/core/modules_v1_test.go(1 hunks)router/core/router.go(5 hunks)router/core/router_config.go(1 hunks)router/internal/utils/ordered_set.go(1 hunks)router/internal/utils/ptrs.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
router/core/router_config.go (1)
router/core/modules_v1.go (1)
ModuleV1(53-61)
router-tests/modules_v1/custom_modules/database_module.go (1)
router/core/modules_v1.go (3)
ModuleV1Info(32-43)ModuleV1(53-61)ModuleV1Context(47-51)
router/core/hooks_context.go (1)
router/core/context.go (1)
RequestContext(66-142)
router/core/hooks.go (2)
router/core/hooks_context.go (18)
ApplicationStartHookContext(4-6)ApplicationStopHookContext(8-10)GraphQLServerStartHookContext(12-14)GraphQLServerStopHookContext(16-18)RouterRequestHookContext(20-22)RouterResponseHookContext(24-26)SubgraphRequestHookContext(28-30)SubgraphResponseHookContext(32-34)OperationPreParseHookContext(36-38)OperationPostParseHookContext(40-42)OperationPreNormalizeHookContext(44-46)OperationPostNormalizeHookContext(48-50)OperationPreValidateHookContext(52-54)OperationPostValidateHookContext(56-58)OperationPrePlanHookContext(60-62)OperationPostPlanHookContext(64-66)OperationPreExecuteHookContext(68-70)OperationPostExecuteHookContext(72-74)router/internal/utils/ordered_set.go (2)
OrderedSet(3-6)NewOrderedSet(9-14)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (32)
router/internal/utils/ptrs.go (1)
3-5: LGTM! Clean generic pointer utility implementation.The generic
Ptrfunction is well-implemented and follows Go conventions. This utility will be helpful for creating pointers to values throughout the codebase, especially in the context of the new ModulesV1 system.router/core/router_config.go (1)
104-104: LGTM! Proper integration of ModulesV1 support in configuration.The addition of
customModulesV1field follows the existing pattern and naming convention. This enables passing custom ModuleV1 instances to the router configuration, which aligns perfectly with the new modular plugin system objectives.router/core/hooks_test.go (2)
13-27: LGTM! Well-designed mock implementation for testing.The
mockHookinterface andmockHookImplstruct provide a clean abstraction for testing hook execution. The ability to simulate failures through theshouldErrflag enables comprehensive error testing scenarios.
29-70: LGTM! Comprehensive test coverage for hook execution.The test suite covers all critical scenarios:
- Success path with multiple hooks
- Error handling and message formatting when hooks fail
- Edge case with empty hook lists
The error message assertion on line 58 validates that the error formatting includes both module ID and hook type, which is crucial for debugging in production.
router-tests/modules_v1/module_lifecycle_test.go (1)
14-36: LGTM! Effective integration test for module lifecycle.This integration test appropriately validates that the new ModulesV1 system works end-to-end without causing regressions. The test:
- Uses a real
DatabaseModuleinstance- Executes an actual GraphQL query
- Verifies expected response data
- Ensures the module lifecycle (provision/cleanup) doesn't interfere with normal operations
The parallel execution and use of
testenvframework follows the project's testing patterns.router-tests/modules_v1/custom_modules/database_module.go (3)
7-7: LGTM! Clean struct definition for the example module.The empty struct is appropriate for this example module that doesn't need to maintain state.
9-18: LGTM! Proper ModuleV1Info implementation.The
Module()method correctly implements the interface requirements:
- Unique module ID "database_module"
- Priority value of 2 for execution ordering
- Factory function that creates new instances
The use of a pointer to the priority value aligns with the interface design allowing for optional priority setting.
20-28: LGTM! Clean lifecycle method implementations.Both
Provision()andCleanup()methods properly:
- Accept the correct
ModuleV1Contextparameter- Use the provided logger for visibility
- Return nil errors for this simple example
- Follow the lifecycle contract defined in the ModuleV1 interface
The logging statements provide good visibility into when the module lifecycle events occur, which will be helpful for debugging and monitoring.
router/internal/utils/ordered_set.go (6)
9-14: LGTM!The constructor properly initializes both the slice and map fields.
17-22: LGTM!The Add method correctly maintains both uniqueness and insertion order.
39-42: LGTM!Efficient O(1) membership check using the map.
44-50: LGTM!Good defensive programming - returning a copy prevents external modification of the internal state.
52-55: LGTM!Simple and correct length method.
57-61: LGTM!Properly resets both the slice and map by creating new instances.
router/core/router.go (5)
86-86: LGTM!Properly typed field for managing ModulesV1 lifecycle hooks.
644-662: LGTM!The method correctly combines custom and globally registered ModulesV1, with proper early return when no modules are present.
939-942: LGTM!ModulesV1 initialization is properly integrated into the bootstrap sequence with appropriate error handling.
1498-1508: LGTM!ModulesV1 cleanup is properly integrated with concurrent execution and appropriate error handling. The context propagation ensures cleanup respects the shutdown timeout.
1763-1767: LGTM!The option function follows the established pattern for router configuration.
router/core/modules_v1_test.go (5)
14-83: Well-designed test modules!The test modules effectively cover various scenarios including error conditions, multiple hook implementations, and panic cases. The interface guards provide compile-time verification.
84-151: Comprehensive registration tests!The tests thoroughly cover all registration scenarios including edge cases and the important hook deduplication behavior when a module implements overlapping interfaces.
153-218: Thorough priority sorting tests!The tests correctly verify that modules are sorted by priority with nil priorities treated as lowest, and registration order is preserved for equal priorities.
247-279: Good cleanup error aggregation test!The test properly verifies that all modules get cleaned up regardless of individual failures, and that errors are correctly aggregated.
281-409: Excellent error handling test coverage!The tests thoroughly verify error propagation, wrapping with module context, and proper error messages for all lifecycle phases.
router/core/hooks_context.go (1)
4-74: Well-designed hook context interfaces!The interfaces follow a consistent pattern and provide excellent type safety and extensibility for the hook system. The separation of contexts for each lifecycle phase allows for future enhancements without breaking existing implementations.
router/core/modules_v1.go (3)
15-31: Thread-safe registry with proper validation!The module registry implementation correctly handles concurrent access and validates all required fields before registration.
Also applies to: 71-88, 108-118
32-43: Well-designed priority system!Using a pointer for Priority elegantly distinguishes between explicit zero priority and unset priority. The sorting implementation correctly handles nil values as lowest priority.
Also applies to: 93-106
190-232: Excellent structured error handling!The ModuleV1Error type provides clear, actionable error messages with proper context about which module and phase failed. The error unwrapping support enables proper error chain inspection.
router/core/hooks.go (4)
8-137: Well-structured hook interface hierarchy.The interface design follows best practices with clear separation of concerns, allowing modules to implement only the hooks they need. The consistent error handling pattern across all hooks is excellent.
145-205: Excellent use of generics and OrderedSet for hook management.The hookRegistry design ensures predictable hook execution order while maintaining type safety through generics. The comprehensive coverage of all hook types is thorough.
207-253: Clean and flexible hook registration design.The generic
registerHookhelper elegantly handles optional interface implementation through type assertion, allowing modules to implement only the hooks they need. The registration methods are well-organized by lifecycle category.
255-265: Robust hook execution with proper error handling.The fail-fast approach with detailed error wrapping (module ID + hook name) provides excellent debugging context. The debug logging strikes a good balance for observability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
router-tests/modules_v1/custom_modules/database_module.go (1)
128-133: Consider tracking goroutines for proper cleanup.The goroutine launched here could continue running after module cleanup, potentially causing issues. For production modules, consider using a context or wait group to ensure all goroutines complete before cleanup.
For a more robust implementation, track active goroutines:
type DatabaseModule struct { // ... existing fields ... wg sync.WaitGroup } // In SimulateQuery: m.wg.Add(1) go func() { defer m.wg.Done() time.Sleep(10 * time.Millisecond) m.mu.Lock() m.metrics.ActiveQueries-- m.mu.Unlock() }() // In Cleanup: m.wg.Wait() // Wait for all queries to complete
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
router-tests/modules_v1/custom_modules/database_module.go(1 hunks)router-tests/modules_v1/module_lifecycle_test.go(1 hunks)router/core/hooks_test.go(1 hunks)router/core/modules_v1.go(1 hunks)router/core/modules_v1_test.go(1 hunks)router/core/router.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- router/core/hooks_test.go
- router/core/router.go
- router/core/modules_v1_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
router-tests/modules_v1/custom_modules/database_module.go (1)
router/core/modules_v1.go (3)
ModuleV1Info(14-25)ModuleV1(62-70)ModuleV1Context(29-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-router
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
router/core/modules_v1.go (1)
144-158: Well-implemented cleanup with proper error aggregation.The cleanup implementation correctly addresses the previous review feedback by ensuring all modules get a chance to cleanup and aggregating errors using
errors.Join.
kaialang
force-pushed
the
kaialang/mod-1-introduce-pluggable-module-registry-and-lifecycle-hooks
branch
from
49dacb9 to
0249480
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
router-tests/modules_v1/module_lifecycle_test.go (1)
85-87: Replace hard-coded sleep with proper synchronization.Using a fixed sleep duration to wait for asynchronous operations is fragile and can lead to flaky tests. Consider polling the metrics until the expected state is reached with a timeout.
- time.Sleep(20 * time.Millisecond) - finalMetrics := dbModule.GetMetrics() - assert.Equal(t, 0, finalMetrics.ActiveQueries, "all queries should be completed") + // Wait for queries to complete with timeout + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + defer cancel() + + for { + select { + case <-ctx.Done(): + t.Fatal("timeout waiting for queries to complete") + default: + finalMetrics := dbModule.GetMetrics() + if finalMetrics.ActiveQueries == 0 { + assert.Equal(t, 0, finalMetrics.ActiveQueries, "all queries should be completed") + return + } + time.Sleep(1 * time.Millisecond) + } + }router-tests/modules_v1/custom_modules/database_module.go (1)
32-41: Factory function should create a new instance.The
Newfactory function returns the existing instance (m) instead of creating a newDatabaseModule. This violates the expected behavior where each call toNew()should return a fresh instance to avoid shared state issues.func (m *DatabaseModule) Module() core.ModuleV1Info { priority := 2 return core.ModuleV1Info{ ID: "database_module", Priority: &priority, New: func() core.ModuleV1 { - return m + return &DatabaseModule{} }, } }router/core/modules_v1.go (1)
107-107: Remove redundant hook registry creation.A new
hookRegistryis created here butcoreModuleHooksalready has one from its constructor. This local variable shadows the field and the freshly created registry here is never used since line 138 assigns the same registry back to the field.func (c *coreModuleHooks) initCoreModuleHooks(ctx context.Context, modules []ModuleV1Info) error { - hookRegistry := newHookRegistry() var instances []ModuleV1 for _, info := range modules {Then update the hook registration calls to use
c.hookRegistryinstead ofhookRegistry:- hookRegistry.AddApplicationLifecycle(moduleInstance, info.ID) - hookRegistry.AddGraphQLServerLifecycle(moduleInstance, info.ID) - hookRegistry.AddRouterLifecycle(moduleInstance, info.ID) - hookRegistry.AddSubgraphLifecycle(moduleInstance, info.ID) - hookRegistry.AddOperationLifecycle(moduleInstance, info.ID) + c.hookRegistry.AddApplicationLifecycle(moduleInstance, info.ID) + c.hookRegistry.AddGraphQLServerLifecycle(moduleInstance, info.ID) + c.hookRegistry.AddRouterLifecycle(moduleInstance, info.ID) + c.hookRegistry.AddSubgraphLifecycle(moduleInstance, info.ID) + c.hookRegistry.AddOperationLifecycle(moduleInstance, info.ID)And remove line 138:
- c.hookRegistry = hookRegistry
🧹 Nitpick comments (1)
router/internal/utils/ordered_set.go (1)
24-36: Document O(n) complexity of Remove operation.The Remove method has O(n) time complexity due to slice rebuilding. Consider documenting this performance characteristic for users of this data structure.
-// Remove deletes elem from the set if it exists, preserving order of other elements. +// Remove deletes elem from the set if it exists, preserving order of other elements. +// This operation has O(n) time complexity due to slice rebuilding. func (s *OrderedSet[T]) Remove(elem T) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
router-tests/modules_v1/custom_modules/database_module.go(1 hunks)router-tests/modules_v1/module_lifecycle_test.go(1 hunks)router/core/hooks.go(1 hunks)router/core/hooks_context.go(1 hunks)router/core/hooks_test.go(1 hunks)router/core/modules_v1.go(1 hunks)router/core/modules_v1_test.go(1 hunks)router/core/router.go(5 hunks)router/core/router_config.go(1 hunks)router/internal/utils/ordered_set.go(1 hunks)router/internal/utils/ptrs.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- router/internal/utils/ptrs.go
- router/core/hooks_context.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
router/core/router_config.go (1)
router/core/modules_v1.go (1)
ModuleV1(62-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build-router
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
router/core/hooks_test.go (1)
1-100: Well-structured hook tests with comprehensive coverage.The test implementation properly covers hook execution scenarios, error handling, and registration logic with clear test cases and appropriate assertions.
router/core/modules_v1_test.go (1)
1-341: Comprehensive test coverage for ModuleV1 system.The test file provides thorough coverage of module lifecycle, error handling, and hook execution with well-structured test cases and proper assertions.
router/core/router.go (5)
87-87: LGTM!The
moduleHooksfield follows the established pattern alongside the existingmodulesfield.
657-671: LGTM!The implementation correctly handles module initialization with proper sorting by priority and error propagation.
948-951: LGTM!The initialization is properly placed after the existing module system and follows consistent error handling patterns.
1507-1517: LGTM!The cleanup implementation follows the established concurrent cleanup pattern with proper error aggregation.
1772-1776: LGTM!The option function follows the established pattern for router configuration options.
router-tests/modules_v1/custom_modules/database_module.go (1)
43-146: LGTM!The module implementation demonstrates good practices:
- Proper mutex usage for thread-safe access to shared state
- Comprehensive initialization and cleanup logic
- Defensive nil checks in getter methods
- Simulated async behavior in
SimulateQueryrouter/core/modules_v1.go (1)
14-106: LGTM!Excellent implementation with:
- Clear interface documentation including concurrency safety guidelines
- Proper priority sorting with nil handling
- Comprehensive error aggregation in cleanup
- Well-structured error types for debugging
Also applies to: 108-143, 144-202
router/core/hooks.go (1)
1-264: LGTM!Well-designed hook system with:
- Comprehensive lifecycle coverage across all router components
- Type-safe generic implementation with proper traceability
- Ordered execution ensuring predictable behavior
- Clear separation of concerns with dedicated interfaces for each hook type
kaialang
changed the base branch from
main
to
topic/kaialang-eng-1063-open-core-module-system-v1
kaialang
requested review from
a team,
Aenimus,
JivusAyrus,
endigma,
thisisnithin and
wilsonrivera
as code owners
kaialang
changed the base branch from
topic/kaialang-eng-1063-open-core-module-system-v1
to
main
kaialang
changed the base branch from
main
to
topic/kaialang-eng-1063-open-core-module-system-v1
|
Please sort out the rebase issue before merging. |
|
Is this meant to be merged into |
kaialang
force-pushed
the
kaialang/mod-1-introduce-pluggable-module-registry-and-lifecycle-hooks
branch
from
ea37abb to
d4d4db5
Compare
It is meant to be merged into the topic branch |
The issue is gone after rebasing. |
kaialang
merged commit fb92790
into
topic/kaialang-eng-1063-open-core-module-system-v1
kaialang
deleted the
kaialang/mod-1-introduce-pluggable-module-registry-and-lifecycle-hooks
branch
4 participants
Motivation and Context
This PR lays the groundwork for the open core module system (ADR: https://github.com/wundergraph/cosmo/pull/1863/files) by introducing:
Core Module system
Hook system
Dedicated Context for each hook
Utilities
Tests
Usage Example
Followups
Add the exact contract of the first pair of hooks and invoke in router execution. Add integration test for it.
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores